Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: fix type mismatch in NodeStyleEventEmitter #3530

Merged
merged 1 commit into from
Apr 7, 2018
Merged

fix: fix type mismatch in NodeStyleEventEmitter #3530

merged 1 commit into from
Apr 7, 2018

Conversation

joemphilips
Copy link
Contributor

Description:
There was a type mismatch in observable/fromEvent.ts which causes tsc compile error when I tried to compile the following.

Observable.fromEvent(new EventEmitter(), "foo")

Error I've got is below

src/my-library/index.ts:41:5 - error TS2352: Type 'EventEmitter' cannot be converted to type 'NodeStyleEventEmitter'.
  Types of property 'addListener' are incompatible.
    Type '(event: string | symbol, listener: (...args: any[]) => void) => EventEmitter' is not comparable to type '(eventName: string, handler: Function) => void'.
      Types of parameters 'listener' and 'handler' are incompatible.
        Type 'Function' is not comparable to type '(...args: any[]) => void'.
          Type 'Function' provides no match for the signature '(...args: any[]): void'.

NOTE: There was also a same problem in stable branch. Please tell me what should I do for it.
https://github.com/ReactiveX/rxjs/blob/stable/src/observable/FromEventObservable.ts

@joemphilips
fix: fix type mismatch in NodeStyleEventEmitter
bce336e 
* change `type` to `interface` since type does not allow returning this
* define its own type for handler in NodeStyleEventEmitter
* cast handler to NodeEventHandler when adding a listener to sourceObj
@benlesh benlesh merged commit 3f51ddd into ReactiveX:master Apr 7, 2018
@benlesh
Copy link
Member

benlesh commented Apr 7, 2018

Thank you, @joemphilips! ☺️

@cartant
Copy link
Collaborator

cartant commented Apr 7, 2018

@joemphilips Regarding your question about the stable branch. Now that your PR has been merged, you could create another branch (named emitter-fix-stable or whatever) based on the stable branch:

git checkout -b emitter-fix-stable stable

Often, you can use git cherry-pick <commit-sha> to apply a commit made to master to your new branch. However, the file structures have changed radically between stable and master and Git appears to be unable to automatically apply this particular commit.

So you should make the required changes to FromEventObservable.ts in your new branch and then create another PR from it that targets the stable branch instead of master (the GitHub New-Pull-Request page lets you specify the base branch). Then your new PR can be merged with stable.

@joemphilips joemphilips mentioned this pull request Apr 9, 2018
Merged
@joemphilips joemphilips deleted the fix-type-mismatch-in-fromEvent branch April 9, 2018 05:09
@joemphilips
Copy link
Contributor Author

@cartant Thanks a lot! I made a separate PR here #3536

@lock
Copy link

lock bot commented Jun 5, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 5, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@IgorMinar IgorMinar IgorMinar approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@joemphilips @benlesh @cartant @IgorMinar